Make rand an optional dependency #674

roee88 · 2021-08-08T06:49:17Z

Which issue does this PR close?

Closes #671

Rationale for this change

Support wasm32-unknown-unknown target in environments without JavaScript.

What changes are included in this PR?

Change rand to be an optional dependency
Add rand as a dev dependency
Updated Building for WASM section in README
Add wasm32-wasi test in CI

Are there any user-facing changes?

Instructions for compiling to wasm32-unknown-unknown changed

roee88 · 2021-08-08T06:50:43Z

.github/workflows/rust.yml

      - name: Build arrow crate
        run: |
          export CARGO_HOME="/github/home/.cargo"
          export CARGO_TARGET_DIR="/github/home/target"
          cd arrow
-          cargo build --features=js --target wasm32-unknown-unknown
+          cargo build --no-default-features --features=csv,ipc,simd --target wasm32-unknown-unknown
+          cargo build --no-default-features --features=csv,ipc,simd --target wasm32-wasi


Specifically for wasm32-wasi there is no real need to exclude test dependencies so cargo build --features=simd --target wasm32-wasi or cargo build --target wasm32-wasi also work. However, I don't see a real reason to include it.

roee88 · 2021-08-08T06:54:36Z

arrow/README.md


 ```toml
 [dependencies]
-arrow = { version = "5.0", default-features = false, features = ["js"] }
+arrow = { version = "5.0", default-features = false, features = ["csv", "ipc", "simd"] }
 ```



side note

When compiling to wasm32-wasi you can also enable the prettyprint feature if you patch prettytable-rs in your Cargo.toml:

[patch.crates-io] prettytable-rs = { git = "https://github.com/phsym/prettytable-rs", branch = "master"}

This is mentioned in polars but I'm not sure if this fits the arrow README so I didn't include it. The arrow2 project recently changed to comfy-table because prettytable-rs is not maintained (see jorgecarleitao/arrow2#251). I don't think that comfy-table compiles to wasm32-wasi but that's minor for now.

See also #656 from @PsiACE -- I am not sure if the same applies to comfy-table

codecov-commenter · 2021-08-08T07:08:06Z

Codecov Report

Merging #674 (df7d0c9) into master (b682ef5) will increase coverage by 0.00%.
The diff coverage is 88.88%.

❗ Current head df7d0c9 differs from pull request most recent head b9938b2. Consider uploading reports for the commit b9938b2 to get more accurate results

@@           Coverage Diff           @@
##           master     #674   +/-   ##
=======================================
  Coverage   82.42%   82.43%           
=======================================
  Files         168      168           
  Lines       47250    47326   +76     
=======================================
+ Hits        38948    39015   +67     
- Misses       8302     8311    +9

Impacted Files	Coverage Δ
parquet/src/data_type.rs	`77.32% <84.61%> (+0.18%)`	⬆️
parquet/src/column/writer.rs	`93.02% <88.05%> (-0.39%)`	⬇️
parquet/src/arrow/arrow_writer.rs	`98.06% <100.00%> (+0.02%)`	⬆️
arrow/src/array/equal_json.rs	`84.92% <0.00%> (-0.29%)`	⬇️
parquet/src/encodings/encoding.rs	`94.48% <0.00%> (-0.20%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b682ef5...b9938b2. Read the comment docs.

houqp

Anyone know what's the reason for including test_utils as the one of the default features?

roee88 · 2021-08-08T07:25:38Z

Anyone know what's the reason for including test_utils as the one of the default features?

Because cargo test will fail without it. I tried to find alternatives but got blocked by rust-lang/cargo#2911.

nevi-me

LGTM, minor change suggested

nevi-me · 2021-08-08T07:51:28Z

arrow/README.md


 ```toml
 [dependencies]
-arrow = { version = "5.0", default-features = false, features = ["js"] }
+arrow = { version = "5.0", default-features = false, features = ["csv", "ipc", "simd"] }


This doesn't clearly articulate what's happening, compared to what's being changed. How about:

"In order to compile Arrow for wasm32-unknown-unknown you will need to disable default features, then include the desired features, but exclude test dependencies (the test_utils feature). For example, use this snippet in your Cargo.toml:"

Updated using your suggested text

Closes apache#671 Signed-off-by: roee88 <[email protected]>

roee88 · 2021-08-08T08:01:15Z

arrow/README.md

@@ -59,11 +59,13 @@ println!("{:?}", array.value(1));

 ## Building for WASM

-In order to compile Arrow for Web Assembly (the `wasm32-unknown-unknown` WASM target), you will likely need to turn off this crate's default features and use the `js` feature.
+Arrow can compile to WebAssembly using the `wasm32-unknown-unknown` and `wasm32-wasi` targets.


FYI wasm32-unknown-emscripten is not supported because of chronotope/chrono#519

We can accommodate it when there's demand

alamb

Thank you @roee88

It might be worth adding / updating the WASM test: https://github.com/apache/arrow-rs/blob/master/.github/workflows/rust.yml#L294-L335 to ensure whatever target you are using continues to work / we don't mess it up with other dependencies. (update I see this was already done)

@nevi-me / @houqp / @roee88 -- do you think this is a change that we should include in arrow-rs 5.2.0 (I plan to create the release candidate this Thursday or Friday) or should we wait for arrow-rs 6.0.0?

alamb · 2021-08-08T10:42:32Z

arrow/Cargo.toml

+# not the core arrow code itself. Be aware that `rand` must be kept as 
+# an optional dependency for supporting compile to wasm32-unknown-unknown 
+# target without assuming an environment containing JavaScript.
+test_utils = ["rand"]


alamb · 2021-08-08T10:44:07Z

arrow/README.md


 ```toml
 [dependencies]
-arrow = { version = "5.0", default-features = false, features = ["js"] }
+arrow = { version = "5.0", default-features = false, features = ["csv", "ipc", "simd"] }
 ```



See also #656 from @PsiACE -- I am not sure if the same applies to comfy-table

houqp · 2021-08-08T19:04:00Z

@nevi-me / @houqp / @roee88 -- do you think this is a change that we should include in arrow-rs 5.2.0 (I plan to create the release candidate this Thursday or Friday) or should we wait for arrow-rs 6.0.0?

Looks like something we should wait for 6.0.0 since removing js feature is a breaking change.

The feature was removed in PR apache#674, but the feature list in the README wasn't updated

The feature was removed in PR #674, but the feature list in the README wasn't updated

Can drop this after rebase on commit fc04931 "Make rand an optional dependency (apache#674)", first released in 6.0.0

github-actions bot added the arrow Changes to the arrow crate label Aug 8, 2021

roee88 commented Aug 8, 2021

View reviewed changes

houqp approved these changes Aug 8, 2021

View reviewed changes

nevi-me approved these changes Aug 8, 2021

View reviewed changes

Make rand an optional dependency

b9938b2

Closes apache#671 Signed-off-by: roee88 <[email protected]>

roee88 force-pushed the optional-rand branch from e099b23 to b9938b2 Compare August 8, 2021 07:58

roee88 commented Aug 8, 2021

View reviewed changes

alamb approved these changes Aug 8, 2021

View reviewed changes

alamb merged commit fc04931 into apache:master Aug 9, 2021

roee88 deleted the optional-rand branch August 9, 2021 11:59

akazukin5151 added a commit to akazukin5151/arrow-rs that referenced this pull request Apr 2, 2023

Remove non-existent feature from README

952cd61

The feature was removed in PR apache#674, but the feature list in the README wasn't updated

akazukin5151 mentioned this pull request Apr 2, 2023

Remove non-existent js feature from README #4000

Closed

tustvold pushed a commit that referenced this pull request Apr 2, 2023

Remove non-existent feature from README (#4001)

e90c9b4

The feature was removed in PR #674, but the feature list in the README wasn't updated

mcheshkov added a commit to cube-js/arrow-rs that referenced this pull request Aug 27, 2024

Add rand to dev-deps

d0685b2

Can drop this after rebase on commit fc04931 "Make rand an optional dependency (apache#674)", first released in 6.0.0

mcheshkov added a commit to cube-js/arrow-rs that referenced this pull request Aug 27, 2024

Add rand to dev-deps

90d2734

Can drop this after rebase on commit fc04931 "Make rand an optional dependency (apache#674)", first released in 6.0.0

mcheshkov added a commit to cube-js/arrow-rs that referenced this pull request Sep 5, 2024

Add rand to dev-deps

d79db21

Can drop this after rebase on commit fc04931 "Make rand an optional dependency (apache#674)", first released in 6.0.0

mcheshkov added a commit to cube-js/arrow-rs that referenced this pull request Sep 5, 2024

Add rand to dev-deps

0b38897

Can drop this after rebase on commit fc04931 "Make rand an optional dependency (apache#674)", first released in 6.0.0

mcheshkov added a commit to cube-js/arrow-rs that referenced this pull request Sep 6, 2024

Add rand to dev-deps

201defc

Can drop this after rebase on commit fc04931 "Make rand an optional dependency (apache#674)", first released in 6.0.0

mcheshkov added a commit to cube-js/arrow-rs that referenced this pull request Sep 11, 2024

Add rand to dev-deps

fd686d8

Can drop this after rebase on commit fc04931 "Make rand an optional dependency (apache#674)", first released in 6.0.0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make rand an optional dependency #674

Make rand an optional dependency #674

roee88 commented Aug 8, 2021

roee88 Aug 8, 2021 •

edited

Loading

roee88 Aug 8, 2021

alamb Aug 8, 2021

codecov-commenter commented Aug 8, 2021 •

edited

Loading

houqp left a comment

roee88 commented Aug 8, 2021

nevi-me left a comment

nevi-me Aug 8, 2021

roee88 Aug 8, 2021

roee88 Aug 8, 2021

nevi-me Aug 8, 2021

alamb left a comment •

edited

Loading

alamb Aug 8, 2021

alamb Aug 8, 2021

houqp commented Aug 8, 2021

Make rand an optional dependency #674

Make rand an optional dependency #674

Conversation

roee88 commented Aug 8, 2021

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

roee88 Aug 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

side note

Choose a reason for hiding this comment

codecov-commenter commented Aug 8, 2021 • edited Loading

Codecov Report

houqp left a comment

Choose a reason for hiding this comment

roee88 commented Aug 8, 2021

nevi-me left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

houqp commented Aug 8, 2021

roee88 Aug 8, 2021 •

edited

Loading

codecov-commenter commented Aug 8, 2021 •

edited

Loading

alamb left a comment •

edited

Loading